Fix nested field missing sub embedding field #913

wdongyu · 2024-09-20T11:25:10Z

Description

Detail description in #909, this pull add empty check before filling in the result

Related Issues

Resolves #909

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

martin-gaievski · 2024-09-23T16:36:54Z

@wdongyu is this PR is work in progress or ready for review? bunch of tests are failing...

wdongyu · 2024-09-23T16:43:30Z

@martin-gaievski will rerun and fix them... :(

yuye-aws · 2024-09-26T07:46:18Z

@wdongyu Can you check failing CIs: https://github.com/opensearch-project/neural-search/actions/runs/10987606139/job/30536408453?pr=913?

wdongyu · 2024-09-26T07:51:59Z

@yuye-aws working in process, I have found the problem and try to reproduce it on my dev box. It's related to the inference text which is null but sent to mlCommonsClientAccessor

yuye-aws · 2024-09-26T08:17:35Z

Great! Looking forward to see your new commits.

wdongyu · 2024-09-26T12:53:38Z

In the last commit, I have done:

Filter out the null entry in createInferenceListForMapTypeInput, so that we can avoid sending null inference text to mlCommonsClientAccessor.
Not filter out the null entry in createInferenceList when sourceValue instanceof List, because if there exists any null entry for List, an IllegalArgumentException will be thrown, I also add a test to confirm that.

private List<String> createInferenceList(Map<String, Object> knnKeyMap) {
   ...
        if (sourceValue instanceof List) {
            texts.addAll(((List<String>) sourceValue));   --> not filter out null entry
        } else if (sourceValue instanceof Map) {
            createInferenceListForMapTypeInput(sourceValue, texts);    --> filter out null entry
        } 
   ...
}

Refine two tests, one is to make sure the failed doc count is corrected, another is to make sure the model is uploaded completely.

Please correct me if I am wrong. @martin-gaievski @yuye-aws

yuye-aws · 2024-09-26T15:43:36Z

Thanks! Will review this PR tomorrow. @martin-gaievski please start the CI workflow.

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

yuye-aws · 2024-09-27T09:46:50Z

src/testFixtures/java/org/opensearch/neuralsearch/BaseNeuralSearchIT.java

@@ -203,6 +203,7 @@ protected void loadModel(final String modelId) throws Exception {
            isComplete = checkComplete(taskQueryResult);
            Thread.sleep(DEFAULT_TASK_RESULT_QUERY_INTERVAL_IN_MILLISECOND);
        }
+        assertTrue(isComplete);


Nice catch!

yuye-aws · 2024-09-27T09:49:53Z

@wdongyu Please run ./gradlew spotlessapply to fix linting isssue

yuye-aws

@wdongyu It is nice of you to implement high-quality ut and it code. Just need to resolve the following comments.

src/test/java/org/opensearch/neuralsearch/processor/TextEmbeddingProcessorTests.java

vibrantvarun · 2024-09-27T15:57:44Z

Triggered checks. I will review the PR today

wdongyu · 2024-09-29T06:44:27Z

Resolve all above conversations @yuye-aws @martin-gaievski @vibrantvarun

yuye-aws · 2024-09-29T07:23:34Z

Good. Taking a look

yuye-aws · 2024-09-29T07:32:53Z

The code looks good to me. Thanks for the contribution @wdongyu . Can @martin-gaievski or @vibrantvarun start the CI workflow?

martin-gaievski · 2024-10-01T16:26:47Z

Changes looks good to me, thank you! @wdongyu can you please rebase on main? you need to have changes from #916 in order to fix the CI. Once that's done and CI is green I'll approve and merge this PR.

vibrantvarun · 2024-10-01T16:41:33Z

Please rebase with main and LGTM, Thanks

Signed-off-by: wangdongyu.danny <[email protected]>

wdongyu · 2024-10-08T09:51:53Z

Rebase done, please start the CI workflow when you are available @martin-gaievski @vibrantvarun.

yuye-aws · 2024-10-08T11:50:04Z

Rebase done, please start the CI workflow when you are available @martin-gaievski @vibrantvarun.

Thank you! CI is running now. Will approve your PR once the CI gets all passed.

wdongyu · 2024-10-08T12:10:33Z

Thank you! CI is running now. Will approve your PR once the CI gets all passed.

Bunch of tests are still failing, most of them are NoClassDefFoundError, and one of them is for testAgainstOldCluster. Am I missing somethings? @yuye-aws

yuye-aws · 2024-10-08T15:36:33Z

Hi @wdongyu ! I do not think you have missed anything. The following error log seems to be a problem from k-NN repo. Might have time to check it tomorrow.

Caused by:
java.lang.ExceptionInInitializerError: Exception java.util.ServiceConfigurationError: org.apache.lucene.codecs.Codec: Provider org.opensearch.knn.index.codec.KNN910Codec.KNN910Codec could not be instantiated [in thread "SUITE-MLCommonsClientAccessorTests-seed#[E12FC70B18037AED]-worker"]
at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:586)
at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:813)
at java.base/java.util.ServiceLoader$ProviderImpl.get(ServiceLoader.java:729)
at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:68)
at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:52)
at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:38)
at org.apache.lucene.codecs.Codec$Holder.<clinit>(Codec.java:45)
... 18 more

yuye-aws · 2024-10-09T02:25:35Z

A PR has been merged into the k-NN repo: opensearch-project/k-NN#2195. Hopefully the k-NN artifact can fix the CI error. I will rerun the CI later.

martin-gaievski · 2024-10-09T23:56:05Z

You need to wait for #927 to be merged, CI will keep failing until then

yuye-aws · 2024-10-11T02:43:06Z

#927 gets merged. Rerunning the CI now.

yuye-aws

All CI checks have passed. Thanks for your patience and contribution @wdongyu !

martin-gaievski

All good, thanks for contribution.

* Adding non empty check before filling in result Signed-off-by: wangdongyu.danny <[email protected]> (cherry picked from commit b15052c)

* Adding non empty check before filling in result Signed-off-by: wangdongyu.danny <[email protected]> (cherry picked from commit b15052c) Co-authored-by: wdongyu <[email protected]>

wdongyu · 2024-10-12T03:23:28Z

With many thanks for all of you.

wdongyu requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, sean-zheng-amazon, model-collapse, zane-neo, ylwu-amzn, jngz-es, vibrantvarun and zhichao-aws as code owners September 20, 2024 11:25

github-actions bot added the bug Something isn't working label Sep 20, 2024

yuye-aws reviewed Sep 27, 2024

View reviewed changes

martin-gaievski added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Sep 28, 2024

github-actions bot added the v2.18.0 label Sep 29, 2024

wdongyu added 5 commits October 8, 2024 17:48

Adding non empty check before filling in result

5ad1ab1

Signed-off-by: wangdongyu.danny <[email protected]>

Update entry in CHANGELOG

63eebf0

Signed-off-by: wangdongyu.danny <[email protected]>

Update tests to return specific number of vectors

0497f4f

Signed-off-by: wangdongyu.danny <[email protected]>

filter out null entry in createInferenceListForMapTypeInput

7ce1115

Signed-off-by: wangdongyu.danny <[email protected]>

change to iterator style and add unchecked SuppressWarnings

de1a736

Signed-off-by: wangdongyu.danny <[email protected]>

wdongyu force-pushed the fix_nested_field_missing_sub_embedding_field branch from d3dae05 to de1a736 Compare October 8, 2024 09:49

yuye-aws approved these changes Oct 11, 2024

View reviewed changes

martin-gaievski approved these changes Oct 11, 2024

View reviewed changes

martin-gaievski merged commit b15052c into opensearch-project:main Oct 11, 2024
37 checks passed

opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 11, 2024

Fix nested field missing sub embedding field (#913)

6133924

* Adding non empty check before filling in result Signed-off-by: wangdongyu.danny <[email protected]> (cherry picked from commit b15052c)

opensearch-trigger-bot bot mentioned this pull request Oct 11, 2024

[Backport 2.x] Fix nested field missing sub embedding field #930

Merged

brianf-aws mentioned this pull request Oct 11, 2024

[documentation] Adds documentation to help fix local Lucene code errors #928

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nested field missing sub embedding field #913

Fix nested field missing sub embedding field #913

wdongyu commented Sep 20, 2024 •

edited

Loading

martin-gaievski commented Sep 23, 2024

wdongyu commented Sep 23, 2024

yuye-aws commented Sep 26, 2024

wdongyu commented Sep 26, 2024

yuye-aws commented Sep 26, 2024

wdongyu commented Sep 26, 2024

yuye-aws commented Sep 26, 2024 •

edited

Loading

yuye-aws Sep 27, 2024

yuye-aws commented Sep 27, 2024

yuye-aws left a comment

vibrantvarun commented Sep 27, 2024

wdongyu commented Sep 29, 2024

yuye-aws commented Sep 29, 2024

yuye-aws commented Sep 29, 2024

martin-gaievski commented Oct 1, 2024

vibrantvarun commented Oct 1, 2024 •

edited

Loading

wdongyu commented Oct 8, 2024

yuye-aws commented Oct 8, 2024

wdongyu commented Oct 8, 2024

yuye-aws commented Oct 8, 2024 •

edited

Loading

yuye-aws commented Oct 9, 2024 •

edited

Loading

martin-gaievski commented Oct 9, 2024

yuye-aws commented Oct 11, 2024

yuye-aws left a comment

martin-gaievski left a comment

wdongyu commented Oct 12, 2024

Fix nested field missing sub embedding field #913

Fix nested field missing sub embedding field #913

Conversation

wdongyu commented Sep 20, 2024 • edited Loading

Description

Related Issues

Check List

martin-gaievski commented Sep 23, 2024

wdongyu commented Sep 23, 2024

yuye-aws commented Sep 26, 2024

wdongyu commented Sep 26, 2024

yuye-aws commented Sep 26, 2024

wdongyu commented Sep 26, 2024

yuye-aws commented Sep 26, 2024 • edited Loading

yuye-aws Sep 27, 2024

Choose a reason for hiding this comment

yuye-aws commented Sep 27, 2024

yuye-aws left a comment

Choose a reason for hiding this comment

vibrantvarun commented Sep 27, 2024

wdongyu commented Sep 29, 2024

yuye-aws commented Sep 29, 2024

yuye-aws commented Sep 29, 2024

martin-gaievski commented Oct 1, 2024

vibrantvarun commented Oct 1, 2024 • edited Loading

wdongyu commented Oct 8, 2024

yuye-aws commented Oct 8, 2024

wdongyu commented Oct 8, 2024

yuye-aws commented Oct 8, 2024 • edited Loading

yuye-aws commented Oct 9, 2024 • edited Loading

martin-gaievski commented Oct 9, 2024

yuye-aws commented Oct 11, 2024

yuye-aws left a comment

Choose a reason for hiding this comment

martin-gaievski left a comment

Choose a reason for hiding this comment

wdongyu commented Oct 12, 2024

wdongyu commented Sep 20, 2024 •

edited

Loading

yuye-aws commented Sep 26, 2024 •

edited

Loading

vibrantvarun commented Oct 1, 2024 •

edited

Loading

yuye-aws commented Oct 8, 2024 •

edited

Loading

yuye-aws commented Oct 9, 2024 •

edited

Loading